[Impeller] construct text frames on UI thread. #45418

jonahwilliams · 2023-09-03T04:08:59Z

Conversion of SkTextBlobs to impeller::TextFrame objects is one of the most expensive operations in display list dispatching. While the rest of the engine and framework makes a reasonable attempt to cache the SkTextBlobs generated during paragraph construction, the design of the dl dispatcher means that these the Impeller backend will always reconstruct all text frames on each frame - even if the display list/picture that contained those text frames was unchanged.

Removing this overhead is one of the goals of #45386 , however this patch is also fairly risky and will be difficult to land. As a more incremental solution, we can instead construct the impeller::TextFrame objects when performing paragraph painting and record them in the display list. This both moves the text frame construction to the UI thread and allows the framework/engine to cache unchanged text frames.

This also does not conflict with the dl_aiks_canvas patch directly, and is fine to land before or after it does. (though I'd argue we should land this first).

To compare the current performance levels, I ran the complex_layout_scroll perf test, since this is fairly text filled. On a Pixel 6 pro. Across several runs this is a fairly consistent ~1ms raster time improvement.

Skia

  "average_frame_build_time_millis": 1.497333333333333,
  "90th_percentile_frame_build_time_millis": 2.038,
  "99th_percentile_frame_build_time_millis": 17.686,
  "worst_frame_build_time_millis": 23.095,
  "missed_frame_build_budget_count": 3,
  "average_frame_rasterizer_time_millis": 5.5078589743589745,
  "stddev_frame_rasterizer_time_millis": 2.226343414420338,
  "90th_percentile_frame_rasterizer_time_millis": 7.481,
  "99th_percentile_frame_rasterizer_time_millis": 19.11,
  "worst_frame_rasterizer_time_millis": 79.799,
  "missed_frame_rasterizer_budget_count": 7,
  "frame_count": 234,
  "frame_rasterizer_count": 234,
  "new_gen_gc_count": 10,
  "old_gen_gc_count": 2,

Impeller (ToT)

  "average_frame_build_time_millis": 1.431575000000001,
 "90th_percentile_frame_build_time_millis": 2.196,
 "99th_percentile_frame_build_time_millis": 14.486,
 "worst_frame_build_time_millis": 23.728,
 "missed_frame_build_budget_count": 2,
 "average_frame_rasterizer_time_millis": 6.536087499999999,
 "stddev_frame_rasterizer_time_millis": 1.9902712500000004,
 "90th_percentile_frame_rasterizer_time_millis": 9.705,
 "99th_percentile_frame_rasterizer_time_millis": 14.727,
 "worst_frame_rasterizer_time_millis": 17.838,
 "missed_frame_rasterizer_budget_count": 1,
 "frame_count": 240,
 "frame_rasterizer_count": 240,
 "new_gen_gc_count": 10,
 "old_gen_gc_count": 2,

Impeller (Patched)

  "average_frame_build_time_millis": 1.4500167364016743,
"90th_percentile_frame_build_time_millis": 2.478,
"99th_percentile_frame_build_time_millis": 14.883,
"worst_frame_build_time_millis": 18.782,
"missed_frame_build_budget_count": 1,
"average_frame_rasterizer_time_millis": 5.023033333333336,
"stddev_frame_rasterizer_time_millis": 1.6445388888888894,
"90th_percentile_frame_rasterizer_time_millis": 7.814,
"99th_percentile_frame_rasterizer_time_millis": 13.497,
"worst_frame_rasterizer_time_millis": 15.008,
"missed_frame_rasterizer_budget_count": 0,
"frame_count": 239,
"frame_rasterizer_count": 240,
"new_gen_gc_count": 8,
"old_gen_gc_count": 0,

jonahwilliams · 2023-09-05T16:14:01Z

A second component to this change is that the public API for getting bitmap/emoji font requires a Skia paragraph object, and so moving the text frame construction allows us to (later) fix that too

matanlurey

LGTM with a few comments.

matanlurey · 2023-09-05T17:11:45Z

display_list/dl_builder.cc

@@ -1302,6 +1302,48 @@ void DisplayListBuilder::DrawTextBlob(const sk_sp<SkTextBlob>& blob,
  SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawTextBlobFlags);
  drawTextBlob(blob, x, y);
 }
+
+void DisplayListBuilder::drawTextFrame(


Does this mean that drawTextBlob will go unused in the Impeller backend?

If so, would you want to (ifdef'd?) make sure it's not used, i.e. UNIMPLEMENTED, similar to the dispatcher?

I'm not sure if we use DisplayListBuilder on Skia ever. FYI @flar

Yes, we do. That was its original purpose and is iteratively becoming its only purpose.

matanlurey · 2023-09-05T17:12:20Z

display_list/dl_builder.cc

+    return;
+  }
+  impeller::Rect bounds = text_frame->GetBounds();
+  SkRect sk_bounds = SkRect::MakeLTRB(bounds.GetLeft(), bounds.GetTop(),


I'm surprised we don't have a conversion for this? If we don't, feel free to resolve.

We do, its just all over the place.

matanlurey · 2023-09-05T17:15:02Z

third_party/txt/src/skia/paragraph_skia.cc

I wonder if we would want to just do a separate class at some point versus having all these switches?

(No changes requested in this PR, more of an open question/possible TODO)

I think that ultimately depends on how many (if any) additional checks we need to add versus how quickly we move through preview for the remaining platforms. I wouldn't think so at this time, but if we have to do much more than splitting into two might make sense. however is still 80% the same code.

At one point DL was supposed to be an agnostic storage format that could be used for any backend, allowing common operations at the "picture" level. There was potential for more value added operations that we never pursued. Meanwhile performance considerations related to conversions have been driving it into custom backend-specific cases.

matanlurey · 2023-09-05T17:16:03Z

third_party/txt/src/skia/paragraph_skia.cc

@@ -180,6 +202,14 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter {
    return path;
  }

+  bool ShouldRenderAsPath(const DlPaint& paint) const {
+    // Text with non-trivial color sources or stroke paint mode should be


Thanks for adding context.

Is this for performance or correctness or both? Could you note that?

Added correctness information.

matanlurey · 2023-09-05T17:16:37Z

third_party/txt/src/skia/paragraph_skia.cc

@@ -79,6 +82,20 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter {
    }
    size_t paint_id = std::get<PaintID>(paint);
    FML_DCHECK(paint_id < dl_paints_.size());
+
+    if (ShouldRenderAsPath(dl_paints_[paint_id])) {


Consider putting this inside the impeller_enabled_ block below, and FML_DCHECK'ing instead.

I think you also want to guard this with IMPELLER_SUPPORTS_RENDERING?

flar · 2023-09-05T20:12:05Z

How expensive are the TextBlobs themselves? If we kept both around then we'd retain the ability to render a DL under both Skia and Impeller, but at the cost of storing both. That isn't our long-term goal, but it keeps some ability to remain backend-agnostic while we get there.

jonahwilliams · 2023-09-05T20:20:14Z

The text blobs themselves won't be expensive to store, once we have them we have them. But DLs isn't backend agnostic, as both images/textures and shaders are backend specific now.

…ine#45418)

…134089) flutter/engine@5903490...8bacc3b 2023-09-06 [email protected] [Impeller] construct text frames on UI thread. (flutter/engine#45418) 2023-09-06 [email protected] Add import for `<unordered_map>` to fix the g3 build (flutter/engine#45471) 2023-09-06 [email protected] Roll Skia from af473004622f to 0a253625a76a (2 revisions) (flutter/engine#45470) 2023-09-05 [email protected] Roll Skia from 1019c10a2d38 to af473004622f (2 revisions) (flutter/engine#45469) 2023-09-05 [email protected] Adds a comment on clang_arm64_apilevel26 toolchain usage (flutter/engine#45467) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This reverts commit 8bacc3b.

Reverts #45418 Some google3 tests are hitting the CHECK I added in the DlSkCanvasDispatcher::drawTextFrame, which indicates that the SkParagraph code likely thinks impeller is enabled, whereas other code might be running with Skia. Perhaps this could happen if its software rendering? It should be a fatal error on startup so we can track this down.

Reverts flutter#45418 Some google3 tests are hitting the CHECK I added in the DlSkCanvasDispatcher::drawTextFrame, which indicates that the SkParagraph code likely thinks impeller is enabled, whereas other code might be running with Skia. Perhaps this could happen if its software rendering? It should be a fatal error on startup so we can track this down.

…ad." (#45910) (#45958) Reverts #45418 Some google3 tests are hitting the CHECK I added in the DlSkCanvasDispatcher::drawTextFrame, which indicates that the SkParagraph code likely thinks impeller is enabled, whereas other code might be running with Skia. Perhaps this could happen if its software rendering? It should be a fatal error on startup so we can track this down. cherry pick for flutter roll Co-authored-by: Jonah Williams <[email protected]>

Reverts #45418 Some google3 tests are hitting the CHECK I added in the DlSkCanvasDispatcher::drawTextFrame, which indicates that the SkParagraph code likely thinks impeller is enabled, whereas other code might be running with Skia. Perhaps this could happen if its software rendering? It should be a fatal error on startup so we can track this down.

[Impeller] construct text frames on UI thread.

83a7f9c

github-actions bot added the e: impeller label Sep 3, 2023

jonahwilliams added 11 commits September 3, 2023 08:35

revert optional changes to color font checking.

cf17b4d

make stroked/gradient text work and add unit tests.

e8ee793

++

a0cf7ee

++

d1bc27e

Merge branch 'main' of github.com:flutter/engine into text_frame_skia_2

09ea39a

fix perf overlay text.

8e91ad1

fix fuchsia compilation

a2f9ad8

update build file

42da42a

++

95c7f23

++

9454209

++

bea2189

jonahwilliams mentioned this pull request Sep 4, 2023

Performance warning while navigating Wonderous app flutter/flutter#133982

Closed

2 tasks

jonahwilliams marked this pull request as ready for review September 5, 2023 16:14

jonahwilliams requested review from matanlurey, dnfield and flar September 5, 2023 16:14

matanlurey approved these changes Sep 5, 2023

View reviewed changes

chinmaygarde assigned jonahwilliams Sep 5, 2023

chinmaygarde changed the title ~~[Impeller] construct text frames on UI thread.~~ [Impeller] Construct text frames on UI thread. Sep 5, 2023

chinmaygarde changed the title ~~[Impeller] Construct text frames on UI thread.~~ [Impeller] construct text frames on UI thread. Sep 5, 2023

jonahwilliams added 2 commits September 5, 2023 14:04

add comment and use IMPELLER_SUPPORTS_RENDERING.

bb4452f

++

91e4d3e

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2023

auto-submit bot merged commit 8bacc3b into flutter:main Sep 6, 2023

engine-flutter-autoroll mentioned this pull request Sep 6, 2023

Roll Flutter Engine from 590349006d23 to 8bacc3b38707 (5 revisions) flutter/flutter#134089

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2023

8bacc3b38 [Impeller] construct text frames on UI thread. (flutter/eng…

eaefb6a

…ine#45418)

jonahwilliams mentioned this pull request Sep 6, 2023

[Impeller] Flutter Gallery DlDispatch is mostly text conversions flutter/flutter#133204

Closed

jonahwilliams deleted the text_frame_skia_2 branch September 6, 2023 18:33

jonahwilliams restored the text_frame_skia_2 branch September 16, 2023 00:47

jonahwilliams added a commit that referenced this pull request Sep 16, 2023

Revert "[Impeller] construct text frames on UI thread. (#45418)"

d2888d0

This reverts commit 8bacc3b.

jonahwilliams mentioned this pull request Sep 16, 2023

Revert "[Impeller] construct text frames on UI thread." #45910

Merged

XilaiZhang mentioned this pull request Sep 18, 2023

[flutter roll cp] Revert "[Impeller] construct text frames on UI thread." (#45910) #45958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] construct text frames on UI thread. #45418

[Impeller] construct text frames on UI thread. #45418

jonahwilliams commented Sep 3, 2023 •

edited

Loading

jonahwilliams commented Sep 5, 2023

matanlurey left a comment

matanlurey Sep 5, 2023

jonahwilliams Sep 5, 2023

flar Sep 5, 2023

matanlurey Sep 5, 2023

jonahwilliams Sep 5, 2023

matanlurey Sep 5, 2023

jonahwilliams Sep 5, 2023

flar Sep 5, 2023

matanlurey Sep 5, 2023

jonahwilliams Sep 5, 2023

matanlurey Sep 5, 2023

dnfield Sep 5, 2023

jonahwilliams Sep 5, 2023

flar commented Sep 5, 2023

jonahwilliams commented Sep 5, 2023

[Impeller] construct text frames on UI thread. #45418

[Impeller] construct text frames on UI thread. #45418

Conversation

jonahwilliams commented Sep 3, 2023 • edited Loading

Skia

Impeller (ToT)

Impeller (Patched)

jonahwilliams commented Sep 5, 2023

matanlurey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar commented Sep 5, 2023

jonahwilliams commented Sep 5, 2023

jonahwilliams commented Sep 3, 2023 •

edited

Loading